Skip to content

Turn on units of measure (BREAKING CHANGE) #6343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 22, 2025
Merged

Turn on units of measure (BREAKING CHANGE) #6343

merged 4 commits into from
Apr 22, 2025

Conversation

nrc
Copy link
Contributor

@nrc nrc commented Apr 16, 2025

This adds checks for UoM and turns off the warnings for using suffixes etc. It changes the way we use the engine to always send commands in mm and never setting the scene units to the default.

@nrc nrc requested a review from jtran April 16, 2025 05:51
Copy link

qa-wolf bot commented Apr 16, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Apr 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Apr 22, 2025 10:04pm

Copy link

codspeed-hq bot commented Apr 16, 2025

CodSpeed Walltime Performance Report

Merging #6343 will improve performances by 28.16%

Comparing nrc-uom-cmds (cc44d6c) with main (941911e)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

⚡ 7 improvements
✅ 85 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
execute_pipe-with-bend 1.2 s 1.1 s +11.09%
digest_big_kitt 536.1 µs 441.5 µs +21.41%
digest_cube 27.6 µs 22.8 µs +20.94%
digest_lsystem 141.2 µs 116.5 µs +21.18%
digest_math 102.8 µs 80.2 µs +28.16%
digest_mike_stress_test 1.8 ms 1.4 ms +27.46%
digest_pipes_on_pipes 1.9 ms 1.6 ms +20.84%

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 88.85496% with 73 lines in your changes missing coverage. Please review.

Project coverage is 85.34%. Comparing base (3d22f6c) to head (05815f1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
rust/kcl-lib/src/std/args.rs 52.72% 26 Missing ⚠️
rust/kcl-lib/src/execution/exec_ast.rs 62.85% 13 Missing ⚠️
rust/kcl-lib/src/execution/types.rs 81.69% 13 Missing ⚠️
rust/kcl-lib/src/std/math.rs 82.81% 11 Missing ⚠️
rust/kcl-lib/src/execution/geometry.rs 86.36% 3 Missing ⚠️
rust/kcl-lib/src/walk/ast_node.rs 0.00% 3 Missing ⚠️
rust/kcl-lib/src/std/patterns.rs 93.75% 2 Missing ⚠️
rust/kcl-lib/src/std/sketch.rs 98.34% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6343      +/-   ##
==========================================
+ Coverage   85.16%   85.34%   +0.17%     
==========================================
  Files         108      108              
  Lines       46436    46437       +1     
==========================================
+ Hits        39547    39630      +83     
+ Misses       6889     6807      -82     
Flag Coverage Δ
rust 85.34% <88.85%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@jtran jtran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a couple minor things. Hard to say if we've caught everything and propagated types everywhere we need to though.

@jtran
Copy link
Collaborator

jtran commented Apr 16, 2025

As a user debugging after this change, I feel like I'm going to want to inspect values to know whether they've converted properly. I.e. what is the runtime value of this variable in some function? It used to be whatever number I passed in. But now it may have converted based on units in an intermediate function before being passed on.

@nrc
Copy link
Contributor Author

nrc commented Apr 16, 2025

As a user debugging after this change, I feel like I'm going to want to inspect values to know whether they've converted properly. I.e. what is the runtime value of this variable in some function? It used to be whatever number I passed in. But now it may have converted based on units in an intermediate function before being passed on.

Yeah! It's actually more complicated than that because numbers might be converted when they're passed to a function or when they're sent to the engine. I'm not really sure how to give users more insight. I really want to have values or at least types on hover but without access to program memory from the LSP or static checking, that isn't possible.

@jtran
Copy link
Collaborator

jtran commented Apr 17, 2025

There's currently a log pane, but we don't use it. What do you think about adding a log() or println() function and returning it from execution?

Copy link

codspeed-hq bot commented Apr 17, 2025

CodSpeed Instrumentation Performance Report

Merging #6343 will not alter performance

Comparing nrc-uom-cmds (05815f1) with main (3d22f6c)

Summary

✅ 54 untouched benchmarks

@nrc
Copy link
Contributor Author

nrc commented Apr 17, 2025

There's currently a log pane, but we don't use it. What do you think about adding a log() or println() function and returning it from execution?

There's already a log function in src/log.rs, but it doesn't use the log pane, it writes to the console (and it's dev-oriented rather than user-oriented). I think it's a good idea though. I wonder if we could include more numbers in the operation view thing?

@jtran
Copy link
Collaborator

jtran commented Apr 17, 2025

Do you mean Operations in the Feature Tree? We've already been talking about showing variable definitions in there #5729 (comment). And the plan has been for a long time to be able to expand function calls to see inside them. We already record the arguments for function calls in Operations. We just haven't gotten to the UI for it yet.

For logs, I was thinking about something more explicit the way you might use println in Rust. Ideally it would appear as the KCL is executing, but a simple implementation would just return all the logs to be displayed at the end.

One complication is logs would need to be merged from parallel executed modules.

@nrc
Copy link
Contributor Author

nrc commented Apr 17, 2025

Do you mean Operations in the Feature Tree? We've already been talking about showing variable definitions in there #5729 (comment). And the plan has been for a long time to be able to expand function calls to see inside them. We already record the arguments for function calls in Operations. We just haven't gotten to the UI for it yet.

Yeah. I don't think we'd need to show variables, just the arguments to std functions and the values sent with the modelling commands. Though I guess variables might be useful for debugging (we already send variable values to the frontend from the interpreter, so that wouldn't be too hard, at least for variables outside of a function).

For logs, I was thinking about something more explicit the way you might use println in Rust. Ideally it would appear as the KCL is executing, but a simple implementation would just return all the logs to be displayed at the end.

Oh like a log function in KCL? Yeah, that would be really useful. Although how we present the data and syntax for interpolation would need a little design.

One complication is logs would need to be merged from parallel executed modules.

Yeah...

@nrc
Copy link
Contributor Author

nrc commented Apr 22, 2025

Cherry-picked #6431

@jessfraz jessfraz merged commit b7385d5 into main Apr 22, 2025
59 checks passed
@jessfraz jessfraz deleted the nrc-uom-cmds branch April 22, 2025 22:58
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 22, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
modeling-app-github-app bot pushed a commit that referenced this pull request Apr 23, 2025
* Turn on uom checks

Signed-off-by: Nick Cameron <[email protected]>

* Convert all lengths to mm for engine calls

Signed-off-by: Nick Cameron <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants